Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implementation of XDP support #215

Merged
merged 4 commits into from
Jun 30, 2023
Merged

Implementation of XDP support #215

merged 4 commits into from
Jun 30, 2023

Conversation

huseyinsaatci
Copy link
Contributor

Implementation of XDP support for issue #208.

@huseyinsaatci huseyinsaatci changed the title Implement XDP support Implementation of XDP support Jun 26, 2023
@bobrik
Copy link
Contributor

bobrik commented Jun 29, 2023

I updated the PR to make it match the rest of the repo a bit better.

The biggest change is the example, where consolidated multiple metrics into one, added IPv6 support and made it's cardinality lower by not including actual addresses.

Here's how the metric loos like now:

# HELP ebpf_exporter_xdp_incoming_packets_total Incoming packets going through xdp by protocol and port
# TYPE ebpf_exporter_xdp_incoming_packets_total counter
ebpf_exporter_xdp_incoming_packets_total{dst_port="0",ip="ipv4",protocol="icmp"} 5
ebpf_exporter_xdp_incoming_packets_total{dst_port="53",ip="ipv4",protocol="udp"} 3
ebpf_exporter_xdp_incoming_packets_total{dst_port="9435",ip="ipv4",protocol="tcp"} 3
ebpf_exporter_xdp_incoming_packets_total{dst_port="9435",ip="ipv6",protocol="tcp"} 7

Let me know if it looks good to you.

@huseyinsaatci
Copy link
Contributor Author

It seems all great but I just have one question.
If I define a SEC part that has one invalid interface before the others like SEC("xdp/enp8s0,asdsd,lo"), the others won't be attached because of the early error. In this scenario, enp8s0 and lo are valid, enp8s0 will be attached but lo won't be because of the invalid one asdsd. Would not it be better if we solved this problem and attach all valid interfaces?

@bobrik
Only allow attaching XDP to one interface at a time
d89652a 
We shouldn't allow attaching to multiple interfaces in one `SEC`,
because it makes `xdp` different and special.

For any other kind we would export a metric like this when it fails to attach:

  # HELP ebpf_exporter_ebpf_program_attached Whether a program is attached
  # TYPE ebpf_exporter_ebpf_program_attached gauge
  ebpf_exporter_ebpf_program_attached{id="329"} 0

  # HELP ebpf_exporter_ebpf_program_info Info about ebpf programs
  # TYPE ebpf_exporter_ebpf_program_info gauge
  ebpf_exporter_ebpf_program_info{config="something",id="329",program="trace_something",tag="bd45628e0edfd896"} 1

For `xdp` we can have partial failures and that's not great.

If only one interface is allowed, a user can have multiple `SEC`s:

```c
SEC("xdp/lo")
int trace_lo(struct xdp_md *ctx) {
    return xdp_trace(ctx);
}

SEC("xdp/lol")
int trace_lol(struct xdp_md *ctx) {
    return xdp_trace(ctx);
}
```

And they will know which one of them fails to attach, if any:

  # HELP ebpf_exporter_ebpf_program_attached Whether a program is attached
  # TYPE ebpf_exporter_ebpf_program_attached gauge
  ebpf_exporter_ebpf_program_attached{id="329"} 1
  ebpf_exporter_ebpf_program_attached{id="330"} 0

  # HELP ebpf_exporter_ebpf_program_info Info about ebpf programs
  # TYPE ebpf_exporter_ebpf_program_info gauge
  ebpf_exporter_ebpf_program_info{config="xdp",id="329",program="trace_lo",tag="bd45628e0edfd896"} 1
  ebpf_exporter_ebpf_program_info{config="xdp",id="330",program="trace_eth0",tag="bd45628e0edfd896"} 0
@bobrik
Copy link
Contributor

bobrik commented Jun 30, 2023

That's a good concern.

I think we shouldn't allow attaching to multiple interfaces in one SEC, because it makes xdp special.

For any other program we would export a metric like this when it fails to attach:

# HELP ebpf_exporter_ebpf_program_attached Whether a program is attached
# TYPE ebpf_exporter_ebpf_program_attached gauge
ebpf_exporter_ebpf_program_attached{id="329"} 0

# HELP ebpf_exporter_ebpf_program_info Info about ebpf programs
# TYPE ebpf_exporter_ebpf_program_info gauge
ebpf_exporter_ebpf_program_info{config="something",id="329",program="trace_something",tag="bd45628e0edfd896"} 1

For xdp we can have partial failures and that's not great.

I added a commit to only allow a single interface in SEC("xdp"). This way a user can have multiple SECs:

SEC("xdp/lo")
int trace_lo(struct xdp_md *ctx) {
    return xdp_trace(ctx);
}

SEC("xdp/lol")
int trace_lol(struct xdp_md *ctx) {
    return xdp_trace(ctx);
}

And they will know which one of them fails to attach, if any:

# HELP ebpf_exporter_ebpf_program_attached Whether a program is attached
# TYPE ebpf_exporter_ebpf_program_attached gauge
ebpf_exporter_ebpf_program_attached{id="329"} 1
ebpf_exporter_ebpf_program_attached{id="330"} 0

# HELP ebpf_exporter_ebpf_program_info Info about ebpf programs
# TYPE ebpf_exporter_ebpf_program_info gauge
ebpf_exporter_ebpf_program_info{config="xdp",id="329",program="trace_lo",tag="bd45628e0edfd896"} 1
ebpf_exporter_ebpf_program_info{config="xdp",id="330",program="trace_eth0",tag="bd45628e0edfd896"} 0

It's less concise, but it's consistent with how other things work.

Does this make sense?

@huseyinsaatci
Copy link
Contributor Author

It's clearer to separate interfaces when I think it this way. It seems all good, we can roll it out.

@bobrik bobrik merged commit ef29d3e into cloudflare:master Jun 30, 2023
@bobrik bobrik mentioned this pull request Jun 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@huseyinsaatci @bobrik